Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

adding new getter for type obj #4197

Merged
merged 14 commits into from
May 25, 2024
Merged

Conversation

Cheukting
Copy link
Contributor

Adding new getter that works like __mro__ and __bases__ in Python.

closes #4192

Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this looks great! Just one pair of changes WRT lifetimes and some docs suggestions...

newsfragments/4197.added.md Outdated Show resolved Hide resolved
src/types/typeobject.rs Outdated Show resolved Hide resolved
src/types/typeobject.rs Outdated Show resolved Hide resolved
src/types/typeobject.rs Outdated Show resolved Hide resolved
src/types/typeobject.rs Outdated Show resolved Hide resolved
.assume_borrowed_or_err(self.py())
}?
.to_owned()
.downcast_into()?;
Copy link
Contributor

@Hooksie Hooksie May 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it necessary to do a safe downcast? As I understand, a tuple is guaranteed here from Python itself. That makes me think we can save the implicit type check and use downcast_into_unchecked

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We went back and forth at the sprints about this. It looks like it might be possible to guarantee it's ok:

$ python
Python 3.12.0 (main, Oct  8 2023, 21:31:51) [GCC 12.3.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> int.__mro__ = 1
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: cannot set '__mro__' attribute of immutable type 'int'
>>> class Foo:
...     pass
... 
>>> Foo.__mro__ = 1
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
AttributeError: attribute '__mro__' of 'type' objects is not writable

Copy link
Contributor

@Hooksie Hooksie May 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just for completeness, I checked the CPython source directly, and yeah, can confirm this must be a tuple. Both have PyTuple_CheckExact guards on them:

static inline void
set_tp_bases(PyTypeObject *self, PyObject *bases)
{
    assert(PyTuple_CheckExact(bases));
    if (self->tp_flags & _Py_TPFLAGS_STATIC_BUILTIN) {
        // XXX tp_bases can probably be statically allocated for each
        // static builtin type.
        assert(_Py_IsMainInterpreter(_PyInterpreterState_GET()));
        assert(self->tp_bases == NULL);
        if (PyTuple_GET_SIZE(bases) == 0) {
            assert(self->tp_base == NULL);
        }
        else {
            assert(PyTuple_GET_SIZE(bases) == 1);
            assert(PyTuple_GET_ITEM(bases, 0) == (PyObject *)self->tp_base);
            assert(self->tp_base->tp_flags & _Py_TPFLAGS_STATIC_BUILTIN);
            assert(_Py_IsImmortal(self->tp_base));
        }
        _Py_SetImmortal(bases);
    }
    self->tp_bases = bases;
}
static inline void
set_tp_mro(PyTypeObject *self, PyObject *mro)
{
    assert(PyTuple_CheckExact(mro));
    if (self->tp_flags & _Py_TPFLAGS_STATIC_BUILTIN) {
        // XXX tp_mro can probably be statically allocated for each
        // static builtin type.
        assert(_Py_IsMainInterpreter(_PyInterpreterState_GET()));
        assert(self->tp_mro == NULL);
        /* Other checks are done via set_tp_bases. */
        _Py_SetImmortal(mro);
    }
    self->tp_mro = mro;
}

Edit: Correction, is indeed enforced as a tuple, but the real enforcement for the Python side is actually downstream from tp_setget:

static int
type_set_bases_unlocked(PyTypeObject *type, PyObject *new_bases, void *context)
{
    // Check arguments
    if (!check_set_special_type_attr(type, new_bases, "__bases__")) {
        return -1;
    }
    assert(new_bases != NULL);

    if (!PyTuple_Check(new_bases)) {
        PyErr_Format(PyExc_TypeError,
             "can only assign tuple to %s.__bases__, not %s",
                 type->tp_name, Py_TYPE(new_bases)->tp_name);
        return -1;
    }
    // ...

and __mro__ has no setter defined at all

    {"__mro__", (getter)type_get_mro, NULL, NULL},

.assume_borrowed_or_err(self.py())
}?
.to_owned()
.downcast_into()?;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as below

@Cheukting
Copy link
Contributor Author

Got it, seems we will roll back to the unsafe way to do it, right? @davidhewitt I will update this PR soon.

Cheukting and others added 7 commits May 23, 2024 15:53
Co-authored-by: David Hewitt <mail@davidhewitt.dev>
Co-authored-by: David Hewitt <mail@davidhewitt.dev>
Co-authored-by: David Hewitt <mail@davidhewitt.dev>
Co-authored-by: David Hewitt <mail@davidhewitt.dev>
Co-authored-by: David Hewitt <mail@davidhewitt.dev>
Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great to me, thanks!

@davidhewitt davidhewitt added this pull request to the merge queue May 24, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks May 24, 2024
@davidhewitt
Copy link
Member

Failure looks genuine:

  error: the item `FfiPtrExt` is imported redundantly
     --> src/types/typeobject.rs:163:17
      |
  3   | use crate::ffi_ptr_ext::FfiPtrExt;
      |     ----------------------------- the item `FfiPtrExt` is already imported here
  ...
  163 |             use crate::ffi_ptr_ext::FfiPtrExt;
      |                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
      |
      = note: `-D unused-imports` implied by `-D warnings`
  
  error: could not compile `pyo3` due to previous error

... seems like some of the FfiPtrExtr imports lower down in this file can probably just be removed now that we've added an import up the top.

@Hooksie
Copy link
Contributor

Hooksie commented May 24, 2024

Thanks for implementing!

@Cheukting
Copy link
Contributor Author

@davidhewitt thanks! Will have a look at the unused imports

.getattr(intern!(self.py(), "__mro__"))
.expect("Cannot get `__mro__` from object.")
.extract()
.expect("Cannot convert to Rust object.");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

very minor: The expect message here is a little awkward IMO. There is no broad "Rust object" that we're trying to convert to, instead we're converting from the PyObject down into PyTuple.

My suggestion would be something like this instead

Unexpected type in __mro__ attribute.

@Hooksie
Copy link
Contributor

Hooksie commented May 24, 2024

Adding some really small feedback on the expect message. Feel free to disregard if it's not important

src/types/typeobject.rs Outdated Show resolved Hide resolved
src/types/typeobject.rs Outdated Show resolved Hide resolved
Cheukting and others added 2 commits May 24, 2024 19:04
Co-authored-by: Matt Hooks <me@matthooks.com>
Co-authored-by: Matt Hooks <me@matthooks.com>
@Cheukting
Copy link
Contributor Author

Thank you @Hooksie

@davidhewitt davidhewitt added this pull request to the merge queue May 25, 2024
Merged via the queue into PyO3:main with commit d21045c May 25, 2024
43 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature request] Directly expose tp_bases, tp_mro, and tp_subclasses on PyType in high-level API
3 participants